-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow DjangoObjectPermissions to use views that define get_queryset #2905
Allow DjangoObjectPermissions to use views that define get_queryset #2905
Conversation
except AssertionError: | ||
# view.get_queryset() didn't find .queryset | ||
queryset = None | ||
queryset = get_queryset_from_view(view) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably prefer for us to just leave the logic in the method itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough
63c84c3
to
ea2bc91
Compare
@tomchristie I addressed your comment. |
ea2bc91
to
fbd8156
Compare
@xordoquy Is there is a chance to have it included in 3.1.2 ? |
Will try to find some time to review it before the release |
Thank you I appreciate. |
try: | ||
queryset = view.queryset | ||
except AttributeError: | ||
queryset = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block looks confusing as it currently stands.
Can't it just be...
try:
queryset = view.get_queryset()
except AttributeError:
queryset = getattr(view, 'queryset', None)
Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that yo can have .get_queryset()
defined but its implementation might raise AttributeError
during execution. We should prevent catching this AttributeError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but its implementation might raise AttributeError during execution.
That's such a crazy edge case that I don't really care about it. I'd prefer clarity of this code, and we'd still be using sensible enough behavior in that case. (Plus we'd expect the error to be raised elsewhere such as when initially using the queryset in the view)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand. I'm going to make the change
I'll put a 3.1.2 label on it speculatively just to keep it surfaced for the moment, but up to @xordoquy if it stays or not. |
fbd8156
to
eb2be3a
Compare
I made the changes. |
@@ -112,7 +112,8 @@ def has_permission(self, request, view): | |||
except AttributeError: | |||
queryset = getattr(view, 'queryset', None) | |||
except AssertionError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why we have an AssertionError
block here, but it looks wrong. Just the first four lines are sufficient:
try:
queryset = view.get_queryset()
except AttributeError:
queryset = getattr(view, 'queryset', None)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly for two reasons:
1 - To honour getattr(view, '_ignore_model_permissions', False)
behaviour. Since some views without .queryset are tolerated.
2 - Because the absence of queryset is raised few lines below with an adhoc error message.
assert queryset is not None, (
'Cannot apply DjangoModelPermissions on a view that '
)
I thought it would be nice to keep it that way, to be consistent with DjangoModelPermissions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We def don't need the AssertionError
catching in either case, but it might also be nicer to move if getattr(view, '_ignore_model_permissions', False): return True
behavior to instead be above the queryset check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. I like it.
@ticosax Looks nearly there. |
eb2be3a
to
031ac2a
Compare
I pushed a new version. |
Allow DjangoObjectPermissions to use views that define get_queryset
✨ |
😅 Thank you |
ace ! |
I'm sorry for the noise. I hope this time it is the right one.
It is a refactoring of #2904, I added explicit support for
APIRoot
with a test.